Figma: People Page Violates The Figma Style Guide #3171 #3274
Conversation
…ation/talawa-admin into develop-postgres
WalkthroughThis pull request introduces a series of modifications across multiple files in the Talawa Admin project. The changes primarily focus on styling updates, dependency management, and minor refactoring of components. Key modifications include updates to CSS variables, adjustments in color schemes, improvements in component styling, and updates to package dependencies. The changes aim to enhance the visual consistency and maintainability of the application, with a particular emphasis on adhering to the Figma design guidelines. Changes
Assessment against linked issues
Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
Our Pull Request Approval ProcessThanks for contributing! Testing Your CodeRemember, your PRs won't be reviewed until these criteria are met:
Our policies make our code better. ReviewersDo not assign reviewers. Our Queue Monitors will review your PR and assign them.
Reviewing Your CodeYour reviewer(s) will have the following roles:
CONTRIBUTING.mdRead our CONTRIBUTING.md file. Most importantly:
Other
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (3)
.husky/pre-commit (1)
Line range hint
1-8: Unauthorized modification of git hooks detected.The pre-commit hook is a sensitive file that affects the entire team's workflow. Modifications to this file should follow the team's change management process.
Please:
- Revert changes to the pre-commit hook
- Discuss the proposed linting strategy changes with the team
- Follow the proper process for modifying shared development tools
🧰 Tools
🪛 GitHub Actions: PR Workflow
[error] Unauthorized modification or deletion of sensitive file
src/components/CheckIn/TableRow.tsx (1)
Line range hint
76-83: Remove code coverage exclusions and add proper tests.The
istanbul ignore nextstatements are bypassing code coverage requirements. Instead of excluding these lines from coverage, we should add proper tests.Please:
- Remove the
istanbul ignore nextstatements- Add unit tests covering the PDF generation logic, including:
- Blob creation
- URL creation
- Window opening
- Success notification
Consider using a test utility like
jest-mock-windowto properly test window operations.🧰 Tools
🪛 GitHub Actions: PR Workflow
[error] Contains code coverage disable statement. Please remove it and add the appropriate tests.
src/components/LeftDrawer/LeftDrawer.tsx (1)
Line range hint
128-131: Inconsistent icon color scheme.The SettingsIcon still uses white stroke color while other icons have been updated to use black.
Apply this fix to maintain consistency:
<SettingsIcon stroke={`${ isActive === true - ? 'var(--bs-white)' + ? 'var(--bs-black)' : 'var(--bs-secondary)' }`} />
🧹 Nitpick comments (6)
src/components/CollapsibleDropdown/CollapsibleDropdown.tsx (1)
46-46: Style updates align with Figma guidelines.The color scheme changes from white to black text improve contrast and follow the style guide.
Consider adding a comment documenting the style guide reference for these color choices to help future maintainers understand the design decisions.
Also applies to: 54-54, 60-60
src/components/LeftDrawerOrg/LeftDrawerOrg.tsx (3)
124-129: Consider using React.Fragment shorthand.The explicit fragment syntax can be simplified.
- <> - <button - className={`${styles.profileContainer} shimmer`} - data-testid="orgBtn" - /> - </> + <button + className={`${styles.profileContainer} shimmer`} + data-testid="orgBtn" + />
177-183: Improve boolean comparison in conditional styling.The strict equality comparison with
trueis redundant.- variant={isActive === true ? 'success' : ''} - style={{ - backgroundColor: isActive === true ? '#EAEBEF' : '', - }} - className={`${ - isActive === true ? 'text-black' : 'text-secondary' - }`} + variant={isActive ? 'success' : ''} + style={{ + backgroundColor: isActive ? '#EAEBEF' : '', + }} + className={`${ + isActive ? 'text-black' : 'text-secondary' + }`}
189-191: Consider extracting color values to CSS variables.Hard-coded color values should be moved to CSS variables for better maintainability and consistency with the Figma style guide.
- isActive === true - ? 'var(--bs-black)' - : 'var(--bs-secondary)' + isActive + ? 'var(--icon-active-color, var(--bs-black))' + : 'var(--icon-inactive-color, var(--bs-secondary))'src/screens/OrganizationPeople/AddMember.tsx (1)
550-550: Consider responsive button width.Setting a fixed 100% width might not be optimal for all screen sizes.
- width: '100%', + width: 'min(100%, 300px)', + margin: '0 auto',src/assets/css/app.css (1)
14073-14103: Custom button styles properly override Bootstrap defaults.The custom styles effectively:
- Override Bootstrap's default success button appearance
- Add smooth transitions for icon colors
- Maintain consistent hover/active states
However, consider adding comments to explain why these overrides are necessary for future maintainers.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (13)
.eslintrc.json(2 hunks).husky/pre-commit(1 hunks)package.json(4 hunks)src/assets/css/app.css(8 hunks)src/components/CheckIn/TableRow.tsx(1 hunks)src/components/CollapsibleDropdown/CollapsibleDropdown.tsx(3 hunks)src/components/LeftDrawer/LeftDrawer.tsx(2 hunks)src/components/LeftDrawerOrg/LeftDrawerOrg.module.css(1 hunks)src/components/LeftDrawerOrg/LeftDrawerOrg.tsx(4 hunks)src/screens/OrgList/OrgList.tsx(1 hunks)src/screens/OrganizationPeople/AddMember.tsx(3 hunks)src/screens/OrganizationPeople/OrganizationPeople.tsx(6 hunks)src/style/app.module.css(9 hunks)
👮 Files not reviewed due to content moderation or server errors (4)
- .eslintrc.json
- package.json
- src/components/LeftDrawerOrg/LeftDrawerOrg.module.css
- src/style/app.module.css
🧰 Additional context used
🪛 GitHub Actions: PR Workflow
.husky/pre-commit
[error] Unauthorized modification or deletion of sensitive file
src/components/CheckIn/TableRow.tsx
[error] Contains code coverage disable statement. Please remove it and add the appropriate tests.
package.json
[error] Unauthorized modification or deletion of sensitive file
.eslintrc.json
[error] Unauthorized modification or deletion of sensitive file
🪛 Biome (1.9.4)
src/style/app.module.css
[error] 519-519: Unknown property is not allowed.
See CSS Specifications and browser specific properties for more details.
To resolve this issue, replace the unknown property with a valid CSS property.
(lint/correctness/noUnknownProperty)
[error] 520-520: Unknown property is not allowed.
See CSS Specifications and browser specific properties for more details.
To resolve this issue, replace the unknown property with a valid CSS property.
(lint/correctness/noUnknownProperty)
🔇 Additional comments (11)
src/components/CollapsibleDropdown/CollapsibleDropdown.tsx (1)
77-77: Consistent styling in sub-targets.The color scheme changes are consistently applied to sub-targets, maintaining visual hierarchy.
Also applies to: 91-91
src/components/LeftDrawer/LeftDrawer.tsx (1)
99-99: Style updates align with Figma guidelines.The color scheme changes for text and RolesIcon follow the style guide.
Also applies to: 107-107, 121-121
src/components/LeftDrawerOrg/LeftDrawerOrg.tsx (1)
14-14: LGTM! Improved CSS modularity.The change from global CSS to CSS modules enhances style encapsulation and prevents naming conflicts.
src/screens/OrganizationPeople/OrganizationPeople.tsx (1)
424-433: LGTM! Improved table styling.The removal of borders aligns with the Figma style guide and creates a cleaner visual presentation.
src/screens/OrgList/OrgList.tsx (1)
Line range hint
365-374: LGTM! Proper access control implementation.The conditional rendering of the create organization button based on
superAdminstatus follows security best practices.src/screens/OrganizationPeople/AddMember.tsx (1)
438-440: LGTM! Improved modal close handling.Direct use of the close function improves code clarity.
src/assets/css/app.css (5)
2481-2483: Form control focus styles aligned with Figma guide.The focus state styles have been updated to use the correct brand colors.
3502-3510: Primary button styles updated for visual consistency.The primary button colors have been updated to use a cohesive blue color scheme, improving visual harmony.
3536-3549: Verify success button color scheme change.The success button has been changed from traditional green to a gray color scheme (#eaebef). While this matches the Figma design, please verify that this change:
- Aligns with user expectations for success/confirmation actions
- Maintains sufficient contrast for accessibility
3587-3591: Danger button styling refined.The danger button now uses a softer red palette while maintaining clear error/danger signaling.
6106-6106: Close button focus state aligned with global focus styles.The close button focus shadow now matches other focus states in the application.
| display: `${styles.profileImage.search('flex') === -1 ? 'flex' : ''}`, | ||
| justifyContent: `${styles.profileImage.search('center') === -1 ? 'center' : ''}`, | ||
| alignItems: `${styles.profileImage.search('center') === -1 ? 'center' : ''}`, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Simplify conditional style checks.
The string search operations for style checks are inefficient and hard to maintain.
- display: `${styles.profileImage.search('flex') === -1 ? 'flex' : ''}`,
- justifyContent: `${styles.profileImage.search('center') === -1 ? 'center' : ''}`,
- alignItems: `${styles.profileImage.search('center') === -1 ? 'center' : ''}`,
+ display: 'flex',
+ justifyContent: 'center',
+ alignItems: 'center',📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| display: `${styles.profileImage.search('flex') === -1 ? 'flex' : ''}`, | |
| justifyContent: `${styles.profileImage.search('center') === -1 ? 'center' : ''}`, | |
| alignItems: `${styles.profileImage.search('center') === -1 ? 'center' : ''}`, | |
| display: 'flex', | |
| justifyContent: 'center', | |
| alignItems: 'center', |
| { | ||
| field: 'rowNumber', | ||
| headerName: '#', | ||
| minWidth: 80, | ||
| align: 'center', | ||
| headerAlign: 'center', | ||
| headerClassName: `${styles.tableHeader}`, | ||
| sortable: false, | ||
| renderCell: (params: GridCellParams) => { | ||
| // Use params.api.getSortedRowIds() or params.rowIndex to generate row numbers | ||
| const sortedRowIds = params.api.getSortedRowIds(); // Get sorted row IDs | ||
| const rowIndex = sortedRowIds.indexOf(params.id); // Find the index of the current row | ||
| return <div>{rowIndex + 1}</div>; | ||
| }, | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Optimize row number calculation.
The current implementation recalculates sorted row IDs for each cell render, which could impact performance with large datasets.
- renderCell: (params: GridCellParams) => {
- const sortedRowIds = params.api.getSortedRowIds(); // Get sorted row IDs
- const rowIndex = sortedRowIds.indexOf(params.id); // Find the index of the current row
- return <div>{rowIndex + 1}</div>;
- },
+ renderCell: (params: GridCellParams) => {
+ return <div>{params.api.getRowIndex(params.id) + 1}</div>;
+ },📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| { | |
| field: 'rowNumber', | |
| headerName: '#', | |
| minWidth: 80, | |
| align: 'center', | |
| headerAlign: 'center', | |
| headerClassName: `${styles.tableHeader}`, | |
| sortable: false, | |
| renderCell: (params: GridCellParams) => { | |
| // Use params.api.getSortedRowIds() or params.rowIndex to generate row numbers | |
| const sortedRowIds = params.api.getSortedRowIds(); // Get sorted row IDs | |
| const rowIndex = sortedRowIds.indexOf(params.id); // Find the index of the current row | |
| return <div>{rowIndex + 1}</div>; | |
| }, | |
| }, | |
| { | |
| field: 'rowNumber', | |
| headerName: '#', | |
| minWidth: 80, | |
| align: 'center', | |
| headerAlign: 'center', | |
| headerClassName: `${styles.tableHeader}`, | |
| sortable: false, | |
| renderCell: (params: GridCellParams) => { | |
| return <div>{params.api.getRowIndex(params.id) + 1}</div>; | |
| }, | |
| }, |
| const [createNewUserModalisOpen1, setCreateNewUserModalIsOpen1] = | ||
| useState(false); | ||
|
|
||
| // Function to open the modal | ||
| function openCreateNewUserModal1(): void { | ||
| setCreateNewUserModalIsOpen(true); | ||
| } | ||
|
|
||
| // Function to close the modal | ||
| function closeCreateNewUserModal1(): void { | ||
| setCreateNewUserModalIsOpen(false); | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consolidate duplicate modal state management.
There are multiple similar modal state management implementations that could be consolidated.
Consider creating a custom hook for modal management:
function useModal(initialState = false) {
const [isOpen, setIsOpen] = useState(initialState);
const open = useCallback(() => setIsOpen(true), []);
const close = useCallback(() => setIsOpen(false), []);
const toggle = useCallback(() => setIsOpen(prev => !prev), []);
return { isOpen, open, close, toggle };
}Then use it like this:
- const [createNewUserModalisOpen1, setCreateNewUserModalIsOpen1] = useState(false);
- function openCreateNewUserModal1(): void {
- setCreateNewUserModalIsOpen(true);
- }
- function closeCreateNewUserModal1(): void {
- setCreateNewUserModalIsOpen(false);
- }
+ const { isOpen: createNewUserModalIsOpen1, open: openCreateNewUserModal1, close: closeCreateNewUserModal1 } = useModal();
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
src/screens/OrganizationPeople/OrganizationPeople.tsx (1)
181-195: 🛠️ Refactor suggestionOptimize row number calculation.
The current implementation recalculates sorted row IDs for each cell render, which could impact performance with large datasets.
Apply this diff to optimize the row number calculation:
- renderCell: (params: GridCellParams) => { - // Use params.api.getSortedRowIds() or params.rowIndex to generate row numbers - const sortedRowIds = params.api.getSortedRowIds(); // Get sorted row IDs - const rowIndex = sortedRowIds.indexOf(params.id); // Find the index of the current row - return <div>{rowIndex + 1}</div>; - }, + renderCell: (params: GridCellParams) => { + return <div>{params.api.getRowIndex(params.id) + 1}</div>; + },
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (4)
.husky/pre-commit(0 hunks)src/screens/EventManagement/EventManagement.tsx(1 hunks)src/screens/OrganizationPeople/AddMember.tsx(5 hunks)src/screens/OrganizationPeople/OrganizationPeople.tsx(7 hunks)
💤 Files with no reviewable changes (1)
- .husky/pre-commit
🧰 Additional context used
🪛 eslint
src/screens/OrganizationPeople/AddMember.tsx
[error] 100-100: Missing return type on function.
(@typescript-eslint/explicit-function-return-type)
[error] 110-110: 'createNewUserModalIsOpen1' is assigned a value but never used.
(@typescript-eslint/no-unused-vars)
[error] 111-111: 'openCreateNewUserModal1' is assigned a value but never used.
(@typescript-eslint/no-unused-vars)
src/screens/OrganizationPeople/OrganizationPeople.tsx
[error] 25-25: 'react-bootstrap' imported multiple times.
(import/no-duplicates)
[error] 26-26: '@mui/icons-material' imported multiple times.
(import/no-duplicates)
🪛 GitHub Check: Performs linting, formatting, type-checking, checking for different source and target branch
src/screens/OrganizationPeople/AddMember.tsx
[failure] 100-100:
Missing return type on function
[failure] 110-110:
'createNewUserModalIsOpen1' is assigned a value but never used
[failure] 111-111:
'openCreateNewUserModal1' is assigned a value but never used
src/screens/OrganizationPeople/OrganizationPeople.tsx
[failure] 25-25:
'/home/runner/work/talawa-admin/talawa-admin/node_modules/react-bootstrap/esm/index.js' imported multiple times
[failure] 26-26:
'/home/runner/work/talawa-admin/talawa-admin/node_modules/@mui/icons-material/esm/index.js' imported multiple times
🪛 GitHub Actions: PR Workflow
src/screens/OrganizationPeople/AddMember.tsx
[error] 2-2: 'Close' is defined but never used
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyse Code With CodeQL (javascript)
🔇 Additional comments (6)
src/screens/EventManagement/EventManagement.tsx (1)
108-114: LGTM! Type safety improvement.The addition of the
Paramstype and its usage withuseParamshook enhances type safety for URL parameters.src/screens/OrganizationPeople/OrganizationPeople.tsx (3)
204-204: LGTM! Improved profile image styling.The changes improve maintainability by:
- Using CSS classes instead of inline styles
- Using the Avatar component as a fallback for missing profile images
Also applies to: 220-220, 223-226
424-433: LGTM! Cleaner DataGrid styling.The removal of borders between rows, cells, and headers creates a cleaner visual presentation.
341-395: LGTM! Enhanced sorting functionality.The new dropdown component provides a better user experience for sorting with:
- Clear options for users, members, and admins
- Consistent styling and accessibility attributes
src/screens/OrganizationPeople/AddMember.tsx (2)
550-550: LGTM! Improved button styling.Setting the button width to 100% improves visual consistency and usability.
438-438:⚠️ Potential issueFix modal close handler.
The modal is using
closeCreateNewUserModal1which is an unused variable.Apply this diff to fix the modal close handler:
- onHide={closeCreateNewUserModal1} // Use the close function directly + onHide={closeCreateNewUserModal}Likely invalid or redundant comment.
| import { Dropdown } from 'react-bootstrap'; | ||
| import { Sort } from '@mui/icons-material'; |
There was a problem hiding this comment.
Remove duplicate imports.
Consolidate the duplicate imports from 'react-bootstrap' and '@mui/icons-material'.
Apply this diff to fix the imports:
-import { Dropdown } from 'react-bootstrap';
-import { Sort } from '@mui/icons-material';Then update the existing imports at the top of the file:
-import { Delete, Search } from '@mui/icons-material';
+import { Delete, Search, Sort } from '@mui/icons-material';
-import { Button, Form } from 'react-bootstrap';
+import { Button, Dropdown, Form } from 'react-bootstrap';📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import { Dropdown } from 'react-bootstrap'; | |
| import { Sort } from '@mui/icons-material'; | |
| // Remove these lines entirely as they are duplicate imports |
🧰 Tools
🪛 eslint
[error] 25-25: 'react-bootstrap' imported multiple times.
(import/no-duplicates)
[error] 26-26: '@mui/icons-material' imported multiple times.
(import/no-duplicates)
🪛 GitHub Check: Performs linting, formatting, type-checking, checking for different source and target branch
[failure] 25-25:
'/home/runner/work/talawa-admin/talawa-admin/node_modules/react-bootstrap/esm/index.js' imported multiple times
[failure] 26-26:
'/home/runner/work/talawa-admin/talawa-admin/node_modules/@mui/icons-material/esm/index.js' imported multiple times
| function useModal(initialState = false) { | ||
| const [isOpen, setIsOpen] = useState(initialState); | ||
| const open = useCallback(() => setIsOpen(true), []); | ||
| const close = useCallback(() => setIsOpen(false), []); | ||
| const toggle = useCallback(() => setIsOpen((prev) => !prev), []); | ||
|
|
||
| return { isOpen, open, close, toggle }; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add return type annotation to useModal hook.
The hook is well-implemented with memoized functions, but it's missing a return type annotation.
Apply this diff to add the return type:
-function useModal(initialState = false) {
+function useModal(initialState = false): {
+ isOpen: boolean;
+ open: () => void;
+ close: () => void;
+ toggle: () => void;
+} {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function useModal(initialState = false) { | |
| const [isOpen, setIsOpen] = useState(initialState); | |
| const open = useCallback(() => setIsOpen(true), []); | |
| const close = useCallback(() => setIsOpen(false), []); | |
| const toggle = useCallback(() => setIsOpen((prev) => !prev), []); | |
| return { isOpen, open, close, toggle }; | |
| } | |
| function useModal(initialState = false): { | |
| isOpen: boolean; | |
| open: () => void; | |
| close: () => void; | |
| toggle: () => void; | |
| } { | |
| const [isOpen, setIsOpen] = useState(initialState); | |
| const open = useCallback(() => setIsOpen(true), []); | |
| const close = useCallback(() => setIsOpen(false), []); | |
| const toggle = useCallback(() => setIsOpen((prev) => !prev), []); | |
| return { isOpen, open, close, toggle }; | |
| } |
🧰 Tools
🪛 eslint
[error] 100-100: Missing return type on function.
(@typescript-eslint/explicit-function-return-type)
🪛 GitHub Check: Performs linting, formatting, type-checking, checking for different source and target branch
[failure] 100-100:
Missing return type on function
| const { | ||
| isOpen: createNewUserModalIsOpen1, | ||
| open: openCreateNewUserModal1, | ||
| close: closeCreateNewUserModal1, | ||
| } = useModal(); |
There was a problem hiding this comment.
Fix unused variables from useModal hook.
The destructured values from useModal are not used in the component, which may indicate incomplete refactoring.
Either:
- Remove the unused hook call:
-const {
- isOpen: createNewUserModalIsOpen1,
- open: openCreateNewUserModal1,
- close: closeCreateNewUserModal1,
-} = useModal();Or:
2. Complete the refactoring by replacing the existing state management:
-const [createNewUserModalisOpen, setCreateNewUserModalIsOpen] = useState(false);
-function openCreateNewUserModal(): void {
- setCreateNewUserModalIsOpen(true);
-}
-function closeCreateNewUserModal(): void {
- setCreateNewUserModalIsOpen(false);
-}
-const toggleCreateNewUserModal = (): void =>
- setCreateNewUserModalIsOpen(!addUserModalisOpen);With the useModal hook:
+const {
+ isOpen: createNewUserModalisOpen,
+ open: openCreateNewUserModal,
+ close: closeCreateNewUserModal,
+ toggle: toggleCreateNewUserModal,
+} = useModal();Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 eslint
[error] 110-110: 'createNewUserModalIsOpen1' is assigned a value but never used.
(@typescript-eslint/no-unused-vars)
[error] 111-111: 'openCreateNewUserModal1' is assigned a value but never used.
(@typescript-eslint/no-unused-vars)
🪛 GitHub Check: Performs linting, formatting, type-checking, checking for different source and target branch
[failure] 110-110:
'createNewUserModalIsOpen1' is assigned a value but never used
[failure] 111-111:
'openCreateNewUserModal1' is assigned a value but never used
|
Please exclude the sensitive files from your PR |
|
but all sentives files were written in .gitignore |
Not the ones you submitted. |
package.json
Outdated
| "typedoc": "^0.26.10", | ||
| "typedoc-plugin-markdown": "^4.2.10", | ||
| "typedoc": "^0.27.0", | ||
| "typedoc-plugin-markdown": "^4.4.1", |
There was a problem hiding this comment.
Exclude this file from the PR. It's not a requirement of the issue
| @@ -78,7 +78,7 @@ | |||
| 'Helvetica Neue', 'Noto Sans', 'Liberation Sans', Arial, sans-serif, | |||
| 'Apple Color Emoji', 'Segoe UI Emoji', 'Segoe UI Symbol', 'Noto Color Emoji'; | |||
| --bs-font-monospace: SFMono-Regular, Menlo, Monaco, Consolas, | |||
| 'Liberation Mono', 'Courier New', monospace; | |||
| --bs-body-color 'Liberation Mono', 'Courier New', monospace; | |||
There was a problem hiding this comment.
- Exclude this file from the PR. It's not a requirement of the issue
- Use CSS exclusively from
src/style/app.module.css,
| @@ -43,21 +43,21 @@ const collapsibleDropdown = ({ | |||
| <> | |||
| <Button | |||
| variant={showDropdown ? 'success' : ''} | |||
| className={showDropdown ? 'text-white' : 'text-secondary'} | |||
There was a problem hiding this comment.
- Don't use hard coded colors
- Make this file use CSS exclusively from
src/style/app.module.css, with the exception of package based CSS. - Delete the CSS file in the directory of this file. Make the file exclusively use CSS from
src/style/app.module.css - Use no CSS in
assets/css/app.css - This will help with future attempts to having Dark Mode
- Use a variable from
src/style/app.module.css. - Use a variable name that describes the function of the color and not the color itself. For example
--search-button-bgis much better than--light-blue
| @@ -43,21 +43,21 @@ const collapsibleDropdown = ({ | |||
| <> | |||
| <Button | |||
| variant={showDropdown ? 'success' : ''} | |||
| className={showDropdown ? 'text-white' : 'text-secondary'} | |||
| className={showDropdown ? 'text-black' : 'text-secondary'} | |||
There was a problem hiding this comment.
- Don't use hard coded colors
- Make this file use CSS exclusively from
src/style/app.module.css, with the exception of package based CSS. - Delete the CSS file in the directory of this file. Make the file exclusively use CSS from
src/style/app.module.css - Use no CSS in
assets/css/app.css - This will help with future attempts to having Dark Mode
- Use a variable from
src/style/app.module.css. - Use a variable name that describes the function of the color and not the color itself. For example
--search-button-bgis much better than--light-blue
| ) : organization == undefined ? ( | ||
| !isProfilePage && ( | ||
| <button | ||
| className={`${styles.profileContainer} ${styles.bgDanger} text-start text-white`} | ||
| className={`${styles.profileContainer} bg-danger text-start text-white`} |
There was a problem hiding this comment.
- Make this file use CSS exclusively from
src/style/app.module.css, with the exception of package based CSS. - Delete the CSS file in the directory of this file. Make the file exclusively use CSS from
src/style/app.module.css - Use no CSS in
assets/css/app.css - This will help with future attempts to having Dark Mode
- Use a variable from
src/style/app.module.css. - Use a variable name that describes the function of the color and not the color itself. For example
--search-button-bgis much better than--light-blue
| } | ||
| variant={isActive === true ? 'success' : ''} | ||
| style={{ | ||
| backgroundColor: isActive === true ? '#EAEBEF' : '', |
There was a problem hiding this comment.
- Don't use hard coded colors
- Make this file use CSS exclusively from
src/style/app.module.css, with the exception of package based CSS. - Delete the CSS file in the directory of this file. Make the file exclusively use CSS from
src/style/app.module.css - Use no CSS in
assets/css/app.css - This will help with future attempts to having Dark Mode
- Use a variable from
src/style/app.module.css. - Use a variable name that describes the function of the color and not the color itself. For example
--search-button-bgis much better than--light-blue
| backgroundColor: isActive === true ? '#EAEBEF' : '', | ||
| }} | ||
| className={`${ | ||
| isActive === true ? 'text-black' : 'text-secondary' |
There was a problem hiding this comment.
- Make this file use CSS exclusively from
src/style/app.module.css, with the exception of package based CSS. - Delete the CSS file in the directory of this file. Make the file exclusively use CSS from
src/style/app.module.css - Use no CSS in
assets/css/app.css - This will help with future attempts to having Dark Mode
- Use a variable from
src/style/app.module.css. - Use a variable name that describes the function of the color and not the color itself. For example
--search-button-bgis much better than--light-blue
| > | ||
| <div className={styles.iconWrapper}> | ||
| <IconComponent | ||
| name={name == 'Membership Requests' ? 'Requests' : name} | ||
| fill={ | ||
| isActive ? 'var(--bs-black)' : 'var(--bs-secondary)' | ||
| isActive === true | ||
| ? 'var(--bs-black)' |
There was a problem hiding this comment.
- Don't use hard coded colors
- Make this file use CSS exclusively from
src/style/app.module.css, with the exception of package based CSS. - Delete the CSS file in the directory of this file. Make the file exclusively use CSS from
src/style/app.module.css - Use no CSS in
assets/css/app.css - This will help with future attempts to having Dark Mode
- Use a variable from
src/style/app.module.css. - Use a variable name that describes the function of the color and not the color itself. For example
--search-button-bgis much better than--light-blue
| @@ -97,7 +97,20 @@ function AddMember(): JSX.Element { | |||
| setCreateNewUserModalIsOpen(!addUserModalisOpen); | |||
|
|
|||
| const [addMember] = useMutation(ADD_MEMBER_MUTATION); | |||
| function useModal(initialState = false) { | |||
There was a problem hiding this comment.
- Make this file use CSS exclusively from
src/style/app.module.css, with the exception of package based CSS. - Delete the CSS file in the directory of this file. Make the file exclusively use CSS from
src/style/app.module.css - Use no CSS in
assets/css/app.css - This will help with future attempts to having Dark Mode
- Use a variable from
src/style/app.module.css. - Use a variable name that describes the function of the color and not the color itself. For example
--search-button-bgis much better than--light-blue
|
|
There are other issues. Let's start with fixing these |
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (5)
src/screens/OrganizationPeople/AddMember.tsx (1)
540-540: Move inline styles to CSS module.Move the width style to
src/style/app.module.cssto maintain consistency and improve maintainability.- width: '100%', + className: styles.fullWidth,Add to
src/style/app.module.css:.fullWidth { width: 100%; }src/style/app.module.css (2)
121-122: Consider simplifying the shadow definition.The multi-line shadow definition could be more maintainable by using separate variables for each shadow component.
- --input-shadow-color: 4px 4px 10px -1px rgb(168, 168, 168, 0.7), - /* Shadow on bottom-right */ 2px 2px 10px -1px rgba(169, 169, 169, 0.7); /* Lighter shadow on bottom-right */ + --input-shadow-primary: 4px 4px 10px -1px rgb(168, 168, 168, 0.7); + --input-shadow-secondary: 2px 2px 10px -1px rgba(169, 169, 169, 0.7); + --input-shadow-color: var(--input-shadow-primary), var(--input-shadow-secondary);
Line range hint
1440-1450: Use CSS variables for breakpoints consistently.The file defines breakpoint variables but doesn't use them consistently in media queries. Consider using the defined variables for better maintainability.
-@media (max-width: 1440px) { +@media (max-width: var(--breakpoint-desktop)) { .contractOrgList { padding-left: calc(250px + 2rem + 1.5rem); } .listBoxOrgList .itemCardOrgList { width: 100%; } }.github/workflows/scripts/eslint_disable_check.py (2)
51-57: Consider enhancing error handling with proper logging.While the error handling is functional, consider using Python's logging module for better error tracking and consistency.
+import logging + +logging.basicConfig(level=logging.INFO) +logger = logging.getLogger(__name__) + def has_eslint_disable(file_path): try: with open(file_path, "r", encoding="utf-8") as file: content = file.read() return bool(eslint_disable_pattern.search(content)) except FileNotFoundError: - print(f"File not found: {file_path}") + logger.error("File not found: %s", file_path) except PermissionError: - print(f"Permission denied: {file_path}") + logger.error("Permission denied: %s", file_path) except (IOError, OSError) as e: - print(f"Error reading file {file_path}: {e}") + logger.error("Error reading file %s: %s", file_path, e)
74-77: Simplify file extension checking.The file extension checking can be made more concise using tuple unpacking.
- if item.endswith((".ts", ".tsx")) and has_eslint_disable(item): - print(f"Error: File {item} contains eslint-disable statements.") - eslint_found = True + if item.endswith((".ts", ".tsx")): + if has_eslint_disable(item): + print(f"Error: File {item} contains eslint-disable statements.") + eslint_found = True
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (7)
.github/workflows/pull-request.yml(1 hunks).github/workflows/scripts/eslint_disable_check.py(4 hunks)matrix.txt(1 hunks)package.json(1 hunks)src/screens/OrganizationPeople/AddMember.tsx(12 hunks)src/screens/OrganizationPeople/OrganizationPeople.tsx(7 hunks)src/style/app.module.css(11 hunks)
✅ Files skipped from review due to trivial changes (1)
- matrix.txt
🚧 Files skipped from review as they are similar to previous changes (1)
- src/screens/OrganizationPeople/OrganizationPeople.tsx
🧰 Additional context used
🪛 yamllint (1.35.1)
.github/workflows/pull-request.yml
[error] 198-198: syntax error: expected , but found ''
(syntax)
🪛 eslint
src/screens/OrganizationPeople/AddMember.tsx
[error] 2-2: 'Check' is defined but never used.
(@typescript-eslint/no-unused-vars)
🪛 Biome (1.9.4)
src/style/app.module.css
[error] 567-567: Unknown property is not allowed.
See CSS Specifications and browser specific properties for more details.
To resolve this issue, replace the unknown property with a valid CSS property.
(lint/correctness/noUnknownProperty)
[error] 568-568: Unknown property is not allowed.
See CSS Specifications and browser specific properties for more details.
To resolve this issue, replace the unknown property with a valid CSS property.
(lint/correctness/noUnknownProperty)
🔇 Additional comments (6)
src/screens/OrganizationPeople/AddMember.tsx (2)
89-106: Well-implemented modal management hook!The
useModalhook is a great addition that:
- Uses proper TypeScript return type annotation
- Implements memoized functions with useCallback
- Follows the single responsibility principle
431-433: Clean modal header implementation!Good simplification of the modal close handler by using the hook's close function directly.
src/style/app.module.css (1)
1-38: Well-structured CSS methodology documentation!The comprehensive documentation clearly outlines:
- Project goals for reducing CSS duplication
- Steps for contributors
- Naming conventions with examples
- List of global classes
This will help maintain consistency and reduce technical debt..github/workflows/scripts/eslint_disable_check.py (2)
41-43: LGTM: Well-constructed regex pattern.The pattern correctly handles all variations of eslint-disable comments including single-line, next-line, and multi-line formats.
101-103: LGTM: Clear and concise argument parser description.The description effectively communicates the script's purpose.
package.json (1)
171-178: This file should be excluded from the PR.As per the previous review comments, this file should not be included in this PR as it's not a requirement of the issue #3171 which focuses on Figma style guide violations.
| @@ -1,5 +1,5 @@ | |||
| import { useLazyQuery, useMutation, useQuery } from '@apollo/client'; | |||
| import { Check, Close, Search } from '@mui/icons-material'; | |||
| import { Check, Search } from '@mui/icons-material'; | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove unused import.
The Check icon from @mui/icons-material is imported but never used in the component.
-import { Check, Search } from '@mui/icons-material';
+import { Search } from '@mui/icons-material';📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import { Check, Search } from '@mui/icons-material'; | |
| import { Search } from '@mui/icons-material'; |
🧰 Tools
🪛 eslint
[error] 2-2: 'Check' is defined but never used.
(@typescript-eslint/no-unused-vars)
| backgroundColor: 'var(--grey-light)', | ||
| color: 'var(--black-color)', |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use CSS variables from app.module.css.
Replace hardcoded color variables with appropriate variables from src/style/app.module.css to maintain consistency with the Figma style guide and support future dark mode implementation.
- backgroundColor: 'var(--grey-light)',
- color: 'var(--black-color)',
+ backgroundColor: 'var(--table-header-bg)',
+ color: 'var(--table-header-text)',📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| backgroundColor: 'var(--grey-light)', | |
| color: 'var(--black-color)', | |
| backgroundColor: 'var(--table-header-bg)', | |
| color: 'var(--table-header-text)', |
| .profileImage { | ||
| display: flex; | ||
| justifycontent: center; | ||
| alignitems: center; | ||
| height: 100%; | ||
| width: 100%; | ||
| } |
There was a problem hiding this comment.
Fix invalid CSS properties.
The properties justifycontent and alignitems are invalid. They should be hyphenated.
.profileImage {
display: flex;
- justifycontent: center;
- alignitems: center;
+ justify-content: center;
+ align-items: center;
height: 100%;
width: 100%;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| .profileImage { | |
| display: flex; | |
| justifycontent: center; | |
| alignitems: center; | |
| height: 100%; | |
| width: 100%; | |
| } | |
| .profileImage { | |
| display: flex; | |
| justify-content: center; | |
| align-items: center; | |
| height: 100%; | |
| width: 100%; | |
| } |
🧰 Tools
🪛 Biome (1.9.4)
[error] 567-567: Unknown property is not allowed.
See CSS Specifications and browser specific properties for more details.
To resolve this issue, replace the unknown property with a valid CSS property.
(lint/correctness/noUnknownProperty)
[error] 568-568: Unknown property is not allowed.
See CSS Specifications and browser specific properties for more details.
To resolve this issue, replace the unknown property with a valid CSS property.
(lint/correctness/noUnknownProperty)
| if: steps.filter-files.outputs.ts_files != '' | ||
| run: | | ||
| python .github/workflows/scripts/eslint_disable_check.py --files ${{ steps.changed-files.outputs.all_changed_files }} | ||
| python .github/workflows/scripts/eslint_disable_check.py --files ${{ steps.filter-files.outputs.ts_files }} |
There was a problem hiding this comment.
Improve conditional execution formatting.
The indentation of the if condition is incorrect, which could cause workflow failures.
- if: steps.filter-files.outputs.ts_files != ''
+ if: ${{ steps.filter-files.outputs.ts_files != '' }}
run: |
python .github/workflows/scripts/eslint_disable_check.py --files ${{ steps.filter-files.outputs.ts_files }}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if: steps.filter-files.outputs.ts_files != '' | |
| run: | | |
| python .github/workflows/scripts/eslint_disable_check.py --files ${{ steps.changed-files.outputs.all_changed_files }} | |
| python .github/workflows/scripts/eslint_disable_check.py --files ${{ steps.filter-files.outputs.ts_files }} | |
| if: ${{ steps.filter-files.outputs.ts_files != '' }} | |
| run: | | |
| python .github/workflows/scripts/eslint_disable_check.py --files ${{ steps.filter-files.outputs.ts_files }} |
| - name: Filter TypeScript files | ||
| id: filter-files | ||
| run: | | ||
| echo "ts_files=$(echo '${{ steps.changed-files.outputs.all_changed_files }}' | tr ',' '\n' | grep -E '\.(ts|tsx)$' | tr '\n' ' ')" >> $GITHUB_OUTPUT | ||
| shell: bash |
There was a problem hiding this comment.
Fix YAML syntax error and improve file filtering.
There's a syntax error in the YAML structure. Additionally, the file filtering could be more robust.
Apply this diff to fix the syntax and improve the filtering:
- name: Filter TypeScript files
id: filter-files
run: |
- echo "ts_files=$(echo '${{ steps.changed-files.outputs.all_changed_files }}' | tr ',' '\n' | grep -E '\.(ts|tsx)$' | tr '\n' ' ')" >> $GITHUB_OUTPUT
+ ts_files=$(echo '${{ steps.changed-files.outputs.all_changed_files }}' | tr ',' '\n' | grep -E '\.(ts|tsx)$' | tr '\n' ' ')
+ if [ -n "$ts_files" ]; then
+ echo "ts_files=$ts_files" >> $GITHUB_OUTPUT
+ fi
shell: bash📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - name: Filter TypeScript files | |
| id: filter-files | |
| run: | | |
| echo "ts_files=$(echo '${{ steps.changed-files.outputs.all_changed_files }}' | tr ',' '\n' | grep -E '\.(ts|tsx)$' | tr '\n' ' ')" >> $GITHUB_OUTPUT | |
| shell: bash | |
| - name: Filter TypeScript files | |
| id: filter-files | |
| run: | | |
| ts_files=$(echo '${{ steps.changed-files.outputs.all_changed_files }}' | tr ',' '\n' | grep -E '\.(ts|tsx)$' | tr '\n' ' ') | |
| if [ -n "$ts_files" ]; then | |
| echo "ts_files=$ts_files" >> $GITHUB_OUTPUT | |
| fi | |
| shell: bash |
🧰 Tools
🪛 yamllint (1.35.1)
[error] 198-198: syntax error: expected , but found ''
(syntax)
Please merge your code with the latest upstream, and resubmit |
What kind of change does this PR introduce?
bugfix, feature, refactoring
3171
Fixes #3171
2025-01-14.15-32-54.mp4
If relevant, did you update the documentation?
no
Summary
it refacors design of menu and components of menu
Does this PR introduce a breaking change?
no
Checklist
CodeRabbit AI Review
Test Coverage
Have you read the contributing guide?
yes
Summary by CodeRabbit
Release Notes
Dependency Updates
gulp-headerdependencytypedocandtypedoc-plugin-markdowneslint-config-react-appoverridessection in dependency managementUI/UX Improvements
.btn-successclassCode Quality
Access Control
Performance